Skip to content

feat(alerts): hold a platform delivery until its source releases it - #123623

Merged
trunk-io[bot] merged 6 commits into
masterfrom
jimmy/platform-delivery-holds
Oct 26, 2026
Merged

trunk-io[bot] merged 6 commits into
masterfrom
jimmy/platform-delivery-holds

Conversation

@jzhu13

@jzhu13 jzhu13 commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • An insight alert with the investigation agent on holds its notification until the agent's verdict, and a safety net sends it if the agent stalls.
  • The shared alerts platform delivers every evaluation as soon as it records it, so these alerts cannot move to it yet.
  • This adds a generic hold to the platform. No source sets one yet; the insight adapter does in a later layer.

Changes

  • A source can put an evaluation's delivery on PlatformAlertOutcome.held_delivery (request, reason, deadline) instead of starting it.
  • record_outcomes writes the hold to a new PlatformAlertDeliveryHold table in the outcome's own transaction. A recorded firing cannot lose its hold.
  • release_hold in the facade releases a hold once, with send or suppress:
    • The first release wins under a row lock.
    • Only a send returns the request, and only to the winning releaser or its retry. The caller starts the delivery with start_deliveries, so the delivery keeps its usual id.
  • reschedule_hold moves an unreleased hold's deadline, for a source that needs a shorter grace once it knows the outcome.
  • deliver_evaluation refuses an evaluation whose hold is unreleased or suppressed, as a second guard.
  • Safety net: alerts-platform-release-expired-holds runs every minute on the delivery queue.
    • It sends up to 200 holds past their deadline, within one activity payload.
    • It re-reads each deadline under the row lock, so a hold rescheduled after the sweep found it stays held.
    • It suppresses the hold of a disabled alert, so turning the alert back on never sends an old firing.
    • It still sends holds of an alert rolled back to its source, because the request records the owner at check time.
    • It starts any released send that nobody marked started 5 minutes after the release. A releaser that fails between the release and the start still delivers.
    • It prunes holds older than the 90-day history TTL.
  • record_hold_delivery_started lets a releaser mark the delivery it started, so the sweep does not start it again.

Warning

A disabled alert's held firing is suppressed, not sent on re-enable. This departs on purpose from the insight stack's own safety net, which skips a disabled alert and sends the firing whenever the alert comes back.

  • New metric alerts_platform_holds_expired_total counts safety-net sends. A rise means a source holds deliveries and does not release them.
  • docs/internal/alerts-platform-temporal.md gets a "Held deliveries" section.
  • Nothing user-visible changes until a source holds a delivery.

Note

A worker on the previous release decodes PlatformAlertOutcome without held_delivery. The source change that sets it must ship at least one deploy after this one.

How did you test this code?

Test rationale:

  • test_delivery_holds.py (new, no existing coverage for holds):
    • The hold is written with its outcome, and a replayed record adds none.
    • One release wins, and only its send returns the request, including a retry from the same releaser.
    • The sweep sends holds past their deadline and released sends nobody started. It keeps a hold rescheduled after the find, suppresses a disabled alert's hold, and prunes old ones.
    • One sweep returns no more requests than fit its payload budget.
  • test_delivery_evaluation.py: one parameterized case shows an unreleased or suppressed hold sends nothing and a released send goes out. Its default configuration id became a UUID, because the guard queries a UUID column.
  • Ran locally: the new tests, test_delivery_evaluation.py, test_temporal_workflows.py, test_temporal_schedule.py, and the full products/alerts_platform suite. 17 platform tests that read history from ClickHouse fail locally because the local test schema predates the source_detail column on master. CI runs them against a fresh schema.
  • Not run: the sweep workflow end to end against a Temporal test server.

Release status

  • No feature flag controls this change

Docs update

Updated docs/internal/alerts-platform-temporal.md.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5

  • Layer I2 of the plan for moving investigating insight alerts onto the alerts platform. It is independent of the investigation-store layer (I3).
  • Skills: /django-migrations, /writing-tests, /code-review high --fix, /simplify, /reviewing-with-coderabbit, /writing-pr-descriptions.
  • /code-review split pruning out of the expiry lookup. /simplify moved from_stored onto Temporal's own converter, tied hold retention to the history TTL, and kept a failed prune from blocking the sweep.
  • PostHog Review findings, all fixed: the safety net now re-reads the deadline under the lock, recovers a send whose start was lost, and bounds its result by payload size.
  • Suppressing a disabled alert's hold, instead of skipping it, is the owner's decision.
  • CodeRabbit CLI, --deep: one finding, fixed. A hold reason longer than its column, or a negative deadline, is now rejected when the hold is built instead of failing the batch record.
  • No duplicate: an open-PR search for delivery holds found nothing.

A source can now hold an evaluation's delivery instead of starting it.
record_outcomes writes the hold in the outcome's transaction, a release
hands the request back to one winner, and a per-minute sweep sends any
hold past its deadline.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jzhu13 jzhu13 self-assigned this Oct 26, 2026
@trunk-io

trunk-io Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

⚠️ Comment density — 4% of added code lines are comments (27 of 673)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
products/alerts_platform/backend/logic/delivery_holds.py 6 207
products/alerts_platform/backend/models/platform_alert.py 6 48
products/alerts_platform/backend/temporal/workflows.py 6 93
products/alerts_platform/backend/facade/contracts.py 5 41
products/alerts_platform/backend/logic/platform_lifecycle.py 2 15
products/alerts_platform/backend/tests/test_delivery_holds.py 2 169

This check does not block merging. It updates on every push and clears when the share drops.

⚠️ Django migration SQL — 1 new migration to review

We've detected new migrations on this PR. Review the SQL output for each migration:

products/alerts_platform/backend/migrations/0020_delivery_hold.py

/home/runner/work/tmp/tool_cache/Python/3.14.7/x64/lib/python3.14/site-packages/anyio/from_thread.py:119: SyntaxWarning: 'return' in a 'finally' block
  return result
2026-10-26T21:17:15.286654Z [error    ] Path must be a valid database or directory containing databases. [posthog.exceptions_capture] pid=6526 tid=140242322107264
Traceback (most recent call last):
  File "/home/runner/work/posthog/posthog/posthog/geoip.py", line 15, in <module>
    geoip: Optional[GeoIP2] = GeoIP2(cache=8)
                              ~~~~~~^^^^^^^^^
  File "/home/runner/work/tmp/tool_cache/Python/3.14.7/x64/lib/python3.14/site-packages/django/contrib/gis/geoip2.py", line 116, in __init__
    raise GeoIP2Exception(
        "Path must be a valid database or directory containing databases."
    )
django.contrib.gis.geoip2.GeoIP2Exception: Path must be a valid database or directory containing databases.
/home/runner/work/tmp/tool_cache/Python/3.14.7/x64/lib/python3.14/site-packages/sshtunnel.py:1040: SyntaxWarning: 'return' in a 'finally' block
  return (ssh_host,
/home/runner/work/tmp/tool_cache/Python/3.14.7/x64/lib/python3.14/site-packages/langchain_core/_api/deprecation.py:27: UserWarning: Core Pydantic V1 functionality isn't compatible with Python 3.14 or greater.
  from pydantic.v1.fields import FieldInfo as FieldInfoV1
System check identified some issues:

WARNINGS:
?: (axes.W001) You are using the django-axes cache handler for login attempt tracking. Your cache configuration is however invalid and will not work correctly with django-axes. This can leave security holes in your login systems as attempts are not tracked correctly. Reconfigure settings.AXES_CACHE and settings.CACHES per django-axes configuration documentation.
?: (staticfiles.W004) The directory '/home/runner/work/posthog/posthog/frontend/dist' in the STATICFILES_DIRS setting does not exist.
BEGIN;
--
-- Create model PlatformAlertDeliveryHold
--
CREATE TABLE "alerts_platformalertdeliveryhold" ("id" uuid NOT NULL PRIMARY KEY, "evaluation_key" varchar(255) NOT NULL, "reason" varchar(64) NOT NULL, "request" jsonb NOT NULL, "held_at" timestamp with time zone NOT NULL, "release_after" timestamp with time zone NOT NULL, "released_at" timestamp with time zone NULL, "release_action" varchar(16) NULL, "released_by" varchar(16) NULL, "delivery_started_at" timestamp with time zone NULL, "configuration_id" uuid NOT NULL, "team_id" integer NOT NULL, CONSTRAINT "platform_alert_delivery_hold_one_per_evaluation" UNIQUE ("configuration_id", "evaluation_key"));
ALTER TABLE "alerts_platformalertdeliveryhold" ADD CONSTRAINT "alerts_platformalert_configuration_id_376c2fa3_fk_alerts_pl" FOREIGN KEY ("configuration_id") REFERENCES "alerts_platformalertconfiguration" ("id") DEFERRABLE INITIALLY DEFERRED;
CREATE INDEX "alerts_platformalertdeliveryhold_configuration_id_376c2fa3" ON "alerts_platformalertdeliveryhold" ("configuration_id");
CREATE INDEX "alerts_platformalertdeliveryhold_team_id_19cc58b1" ON "alerts_platformalertdeliveryhold" ("team_id");
CREATE INDEX "platform_alert_hold_due_idx" ON "alerts_platformalertdeliveryhold" ("release_after") WHERE "released_at" IS NULL;
CREATE INDEX "platform_alert_hold_start_idx" ON "alerts_platformalertdeliveryhold" ("released_at") WHERE ("delivery_started_at" IS NULL AND "release_action" = 'send');
CREATE INDEX "platform_alert_hold_age_idx" ON "alerts_platformalertdeliveryhold" ("held_at");
COMMIT;

Last updated: 2026-10-26 21:18 UTC (f7919aa)

✅ Django migration risk — migration analysis complete

We've analyzed your migrations for potential risks.

Summary: 1 Safe | 0 Needs Review | 0 Blocked

✅ Safe

Brief or no lock, backwards compatible

alerts_platform.0020_delivery_hold
  └─ #1 ✅ CreateModel
     Creating new table is safe
     model: PlatformAlertDeliveryHold
  │
  └──> ℹ️  INFO:
       ℹ️  Skipped operations on newly created tables (empty tables
       don't cause lock contention).

Last updated: 2026-10-26 21:18 UTC (f7919aa)

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 806a145: 3 Should fix.

This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.

See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.

jzhu13 and others added 2 commits October 26, 2026 12:45
Register the hold model in the IDOR semgrep rules. The sweep now
re-reads the deadline under the row lock, suppresses the hold of a
disabled alert instead of sending it on re-enable, starts released sends
whose start was lost, and keeps its result within one payload.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Defer the Temporal converter and payload imports used by held
deliveries, because the alerts_platform models load the contracts
module during django.setup().

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@trunk-io

trunk-io Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@jzhu13
jzhu13 marked this pull request as ready for review October 26, 2026 20:31
@parameterai

parameterai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Risk: No findings

This increment refines the delivery-hold safety net: the sweep now skips rows locked by other transactions, reuses delivery workflow ids with ALLOW_DUPLICATE_FAILED_ONLY so a completed delivery is never re-run, and the expired-hold metric counts only fresh deadline releases. I found no new security issues; the reuse-policy change in particular tightens double-send behavior rather than loosening it.

Sentinel reviewed 1f2b162 · Review settings

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team October 26, 2026 20:33

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 9b09f78: 3 Should fix.

jzhu13 and others added 3 commits October 26, 2026 13:40
The sweep restarts lost delivery starts with ALLOW_DUPLICATE_FAILED_ONLY,
so a completed delivery does not run again. It skips holds another
transaction has locked, and counts only holds it released at their
deadline as expired.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The prune deletes holds older than their history by id, across teams,
with no request or team input. Mark it for idor-lookup-without-team.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jzhu13 jzhu13 added the stamphog Request AI approval (no full review) label Oct 26, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

The risky changes have owning-team and STRONG-familiarity authorship assurance, with targeted tests. No specific unresolved review concerns or policy refusal grounds are evidenced; the listed formal reviews are on older commits.

  • Author wrote 75% of the modified lines and has 29 merged PRs in these paths (familiarity STRONG).
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 673L, 15F substantive, 931L/19F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1d-complex (931L, 19F, cross-cutting, feat)
stamphog 2.6.1 .stamphog/policy.yml @ f7919aa · reviewed head f7919aa

@jzhu13

jzhu13 commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

/trunk merge

@trunk-io
trunk-io Bot merged commit 5e0214a into master Oct 26, 2026
271 of 273 checks passed
@trunk-io
trunk-io Bot deleted the jimmy/platform-delivery-holds branch October 26, 2026 23:14
@deployment-status-posthog

deployment-status-posthog Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-10-26 23:42 UTC Run
prod-us ✅ Deployed 2026-10-26 23:57 UTC Run
prod-eu ✅ Deployed 2026-10-26 23:57 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant